Skip to content

move giscus smoke tests to quarto-dev/quarto-cli #13319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

move giscus smoke tests to quarto-dev/quarto-cli #13319

wants to merge 1 commit into from

Conversation

@gordonwoodhull
Copy link
Contributor

@gordonwoodhull gordonwoodhull commented Sep 2, 2025 •
edited
Loading

Ref #13298

These seem to be just smoke tests and they only need a valid, giscus-enabled repo.

I'm not clear how these are tested. The repo change in the configuration makes them preview ok!

PR to see if this clears CI.

Draft because I don't understand how this is tested!

@gordonwoodhull
move giscus smoke tests to quarto-dev/quarto-cli
39b43c0 
they just need a valid, giscus-enabled repo here
@posit-snyk-bot
Copy link
Collaborator

posit-snyk-bot commented Sep 2, 2025 •
edited
Loading

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

@gordonwoodhull gordonwoodhull mentioned this pull request Sep 2, 2025
Merged
@cderv
Copy link
Collaborator

cderv commented Sep 3, 2025

I must admit that I don't know either... I can't find back the CI log with the issue, and I can't reproduce anymore. So I don't know really what those tests do or why they would fail... 🤔

@cderv
Copy link
Collaborator

cderv commented Sep 3, 2025 •
edited
Loading

And I found back the error I have seen

[smoke] > quarto render docs\smoke-all\issues\4820-giscus-dark-mode\lightly-light-cobalt\index.qmd --to html ... FAILED (21s)
Uncaught error from ./smoke/smoke-all.test.ts FAILED

 ERRORS 

[smoke] > quarto render docs\smoke-all\issues\4820-giscus-dark-mode\lightly-light-cobalt\index.qmd --to html => ./test.ts:329:8
error: AssertionError: Failed assertion: 

--------------------------------------------------------------------------------
[smoke] > quarto render docs\smoke-all\issues\4820-giscus-dark-mode\lightly-light-cobalt\index.qmd --to html
          run-tests.ps1 smoke\smoke-all.test.ts

[verify] > No Errors or Warnings

Error or Warnings During Execution
|TypeError: error sending request for url (https://giscus.app/api/discussions/categories?repo=allenmanning/giscus-examples): client error (Connect): tcp connect error: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. (os error 10060): A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. (os error 10060)

Stack trace:
    at async mainFetch (ext:deno_fetch/26_fetch.js:170:12)
    at async fetch (ext:deno_fetch/26_fetch.js:392:7)
    at async getGithubDiscussionsMetadata (file:///D:/a/quarto-cli/quarto-cli/src/core/giscus.ts:38:20)
    at async htmlFormatExtras (file:///D:/a/quarto-cli/quarto-cli/src/format/html/format-html.ts:589:30)
    at async Object.formatExtras (file:///D:/a/quarto-cli/quarto-cli/src/format/html/format-html.ts:207:11)
    at async runPandoc (file:///D:/a/quarto-cli/quarto-cli/src/command/render/pandoc.ts:526:10)
    at async renderPandoc (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render.ts:202:24)
    at async Object.onRender (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render-files.ts:720:30)
    at async renderFileInternal (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render-files.ts:682:9)
    at async renderFiles (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render-files.ts:330:9)|
AssertionError: Error or Warnings During Execution
|TypeError: error sending request for url (https://giscus.app/api/discussions/categories?repo=allenmanning/giscus-examples): client error (Connect): tcp connect error: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. (os error 10060): A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. (os error 10060)

Stack trace:
    at async mainFetch (ext:deno_fetch/26_fetch.js:170:12)
    at async fetch (ext:deno_fetch/26_fetch.js:392:7)
    at async getGithubDiscussionsMetadata (file:///D:/a/quarto-cli/quarto-cli/src/core/giscus.ts:38:20)
    at async htmlFormatExtras (file:///D:/a/quarto-cli/quarto-cli/src/format/html/format-html.ts:589:30)
    at async Object.formatExtras (file:///D:/a/quarto-cli/quarto-cli/src/format/html/format-html.ts:207:11)
    at async runPandoc (file:///D:/a/quarto-cli/quarto-cli/src/command/render/pandoc.ts:526:10)
    at async renderPandoc (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render.ts:202:24)
    at async Object.onRender (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render-files.ts:720:30)
    at async renderFileInternal (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render-files.ts:682:9)
    at async renderFiles (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render-files.ts:330:9)|
    at Module.assert (https://jsr.io/@std/assert/0.224.0/assert.ts:18:11)
    at assert (https://jsr.io/@std/testing/0.224.0/asserts.ts:608:11)
    at Object.verify (file:///D:/a/quarto-cli/quarto-cli/tests/verify.ts:166:7)
    at fn (file:///D:/a/quarto-cli/quarto-cli/tests/test.ts:240:25)
    at eventLoopTick (ext:core/01_core.js:175:7)
    at async innerWrapped (ext:cli/40_test.js:191:5)
    at async outerWrapped (ext:cli/40_test.js:134:14)

OUTPUT:
    TypeError: error sending request for url (https://giscus.app/api/discussions/categories?repo=allenmanning/giscus-examples): client error (Connect): tcp connect error: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. (os error 10060): A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. (os error 10060)
    
    Stack trace:
        at async mainFetch (ext:deno_fetch/26_fetch.js:170:12)
        at async fetch (ext:deno_fetch/26_fetch.js:392:7)
        at async getGithubDiscussionsMetadata (file:///D:/a/quarto-cli/quarto-cli/src/core/giscus.ts:38:20)
        at async htmlFormatExtras (file:///D:/a/quarto-cli/quarto-cli/src/format/html/format-html.ts:589:30)
        at async Object.formatExtras (file:///D:/a/quarto-cli/quarto-cli/src/format/html/format-html.ts:207:11)
        at async runPandoc (file:///D:/a/quarto-cli/quarto-cli/src/command/render/pandoc.ts:526:10)
        at async renderPandoc (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render.ts:202:24)
        at async Object.onRender (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render-files.ts:720:30)
        at async renderFileInternal (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render-files.ts:682:9)
        at async renderFiles (file:///D:/a/quarto-cli/quarto-cli/src/command/render/render-files.ts:330:9)
    throw new AssertionError(msg);
          ^
    at assert (https://jsr.io/@std/assert/0.224.0/assert.ts:18:11)
    at Module.fail (https://jsr.io/@std/assert/0.224.0/fail.ts:17:3)
    at fail (https://jsr.io/@std/testing/0.224.0/asserts.ts:663:11)
    at fn (file:///D:/a/quarto-cli/quarto-cli/tests/test.ts:303:11)

./smoke/smoke-all.test.ts (uncaught error)
error: (in promise) TypeError: error sending request for url (https://giscus.app/api/discussions/categories?repo=allenmanning/giscus-examples): client error (Connect): tcp connect error: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. (os error 10060): A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. (os error 10060)
  const response = await fetch(url);
                   ^
    at async mainFetch (ext:deno_fetch/26_fetch.js:170:12)
    at async fetch (ext:deno_fetch/26_fetch.js:392:7)
    at async getGithubDiscussionsMetadata (file:///D:/a/quarto-cli/quarto-cli/src/core/giscus.ts:38:20)
    at async resolveFormatForGiscus (file:///D:/a/quarto-cli/quarto-cli/src/project/types/website/website-giscus.ts:65:24)
This error was not caught from a test and caused the test runner to fail on the referenced module.
It most likely originated from a dangling promise, event/timeout handler or top-level code.

Oddly it was only on windows: https://github.com/quarto-dev/quarto-cli/actions/runs/17297423070/job/49174379603

It seems we call metadata for giscus from the repo in
https://github.com/quarto-dev/quarto-cli/blob/main/src/project/types/website/website-giscus.ts

SO the smoke-all test may just test that this does not fail 🤷‍♂️

@cderv
Copy link
Collaborator

cderv commented Sep 3, 2025

While doing the test with

website:
  comments:
    giscus:
      repo: allenmanning/giscus-examples

we do get to

quarto-cli/src/project/types/website/website-giscus.ts

Lines 70 to 76 in 4bba7f4

} catch {
// Couldn't read the cached metadata, fetch it
removeIfExists(path);
giscusMeta = await getGithubDiscussionsMetadata(repo);
const jsonStr = JSON.stringify(giscusMeta, undefined, 2);
Deno.writeTextFileSync(path, jsonStr);
}

but this fetch leads to error we don't even catch

quarto-cli/src/project/types/website/website-giscus.ts

Line 73 in 4bba7f4

giscusMeta = await getGithubDiscussionsMetadata(repo); 

> giscusMeta
{
  error: "giscus is not installed on this repository",
}

We don't handle non configured repo at this stage

quarto-cli/src/core/giscus.ts

Lines 32 to 41 in 4bba7f4

export async function getGithubDiscussionsMetadata(repo: string) {
const url = encodeURI(
`https://giscus.app/api/discussions/categories?repo=${repo}`,
);
// Fetch repo info
const response = await fetch(url);
const jsonObj = await response.json();
return jsonObj as GithubDiscussionMetadata;
}

we probably should... 🤔

@cderv
Copy link
Collaborator

cderv commented Sep 3, 2025

Maybe we could do

diff --git a/src/core/giscus.ts b/src/core/giscus.ts
index add751582..1113f7e7e 100644
--- a/src/core/giscus.ts
+++ b/src/core/giscus.ts
@ -36,6 +35,11 @@ export async function getGithubDiscussionsMetadata(repo: string) {

   // Fetch repo info
   const response = await fetch(url);
+  if (!response.ok) {
+    throw new Error(
+      `Failed to fetch discussions metadata: ${response.statusText} from ${repo}. Check giscus repo setting, and check this repo is correctly configured with giscus.`,
+    );
+  }
   const jsonObj = await response.json();
   return jsonObj as GithubDiscussionMetadata;
 }

And then test an unconfigured repo to check it errors, and a configured repo to check it does not error and get the right content.

Initially, the smoke-all tests were added for

And maybe we do test giscus light / dark already, and better using playwright. I remember something like this...

@gordonwoodhull
Copy link
Contributor Author

gordonwoodhull commented Sep 3, 2025 •
edited
Loading

I see... I didn't realize there was a render-time fetch for giscus. That makes much more sense!

And it was only failing for Windows, which explains why I was unable to repro locally.

Okay, yeah this "fix" is inadequate and I agree we need better error handling to get consistent results.

Now that I think of it, I'm actually not sure if quarto-dev/quarto-cli is valid... quarto-dev/quarto-web is the one where we have giscus enabled.

Okay, I'll close this for now. Too much of a rabbit hole and I have slides to prepare. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gordonwoodhull @posit-snyk-bot @cderv